-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Access single file or folder from IStorageFolder by name #17771
Access single file or folder from IStorageFolder by name #17771
Conversation
I was not able to have an Android-Implementation that does not iterate trough the whole folder to find the first Item with the given name. The other thing is, that I don't have an IOS device to test the IOS-implementation. |
Questions:
|
We'll discuss that in an API review meeting soon and let you know, but I think the return type should be nullable. After all, even the
Platform specific code is unfortunately hard to test, and we don't currently have integration tests for storage, so manual testing should be ok here.
Adding new members to an interface is a breaking change, which is what the CI build reports here. However, You should run |
Diff for API review: namespace Avalonia.Platform.Storage {
{
[NotClientImplementable]
public interface IStorageFolder : IStorageItem
{
+ System.Threading.Tasks.Task<IStorageFile?> GetFileAsync(System.String name);
+ System.Threading.Tasks.Task<IStorageFolder?> GetFolderAsync(System.String name);
}
} |
Thank you for your response. I'm still down to finish this MR so I will wait until the API review. |
Notes from the API review meeting: The API shape is good. However, all platforms should behave the same, as much as possible. |
8121655
to
0535cb6
Compare
0535cb6
to
3f449a4
Compare
Thanks. I incorporated the browser change and updated the api-diff. It seems a bit weird because throwing vs. returning null is not everywhere strictly enforced when using the nullable operator. Should I just catch all Exceptions and return null? Event in the browser example, while I return null (if it is not a directory), there is still the potential to throw an Exception (wrong access, etc. ). But for the most part is shouldn't. At @MrJul: Are you able to review my changes? |
You can test this PR using the following package version. |
We should model this on the Other implementations should match this behavior. Return |
Got it and also re-tested the browser implementation. There where also some other JS-errors throws that I needed to suppress (NotFound and TypeMissmatch) to match the behavior of the other implementation. |
You can test this PR using the following package version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
What does the pull request do?
This PR addresses #17276. It allows to access a single file or folder by name given you have already a folder (
IStorageFolder
).What is the current behavior?
You can currently archive this by iteration over the whole folder with
GetItemsAsync()
and pick the file with the given name. This has performance considerations and therefore, when applicable, the implementations tries to access the file directly.What is the updated/expected behavior with this PR?
When you get a folder (
IStorageFolder
) e.g through a file-picker, you can access the child file/folder by name directly, returningIStorageFile
orIStorageFolder
depending on the called method.How was the solution implemented (if it's not obvious)?
The interface
IStorageFolder
was extended as follows:Therefore, an implementation for the following platforms was provided:
Testing
Tested it on following platforms:
Extended the ControlCatalog for testing:
Checklist
Breaking changes
No breaking changes, this only extends the public interface
Fixed issues
Fixes #17276